Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move label values json aggregation into Java #1977

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

lampajr
Copy link
Member

@lampajr lampajr commented Sep 11, 2024

Changes proposed

I am proposing to move part of the /labelValues logic from jdbc to Java, especially the json aggregation.

This seems coming with a huge improvement, here some results I obtained:

Starting from empty db

Before
image

After
image

With prod backup and huge response (~100MB)

Before
image

After
image

Open points

  • This PR is still in draft as with this change the filter and multiFilter does not work anymore as we don't have the whole object in the query anymore.. therefore we should think how we can fix that.
  • Fix filter when it is a simple JSON path string
  • Fix ordering by labels As agreed this won't be supported any longer

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@lampajr
Copy link
Member Author

lampajr commented Sep 11, 2024

This PR is still in draft as with this change the filter and multiFilter does not work anymore as we don't have the whole object in the query anymore.. therefore we should think how we can fix that.

As expected these tests are failing:

Error:  Failures: 
Error:    TestServiceTest.labelValuesFilterMultiSelectBoolean:582 1 expectation failed.
Expected status code <200> but was <500>.

Error:    TestServiceTest.labelValuesFilterMultiSelectNumber:555 1 expectation failed.
Expected status code <200> but was <500>.

Error:    TestServiceTest.labelValuesFilterMultiSelectStrings:528 1 expectation failed.
Expected status code <200> but was <500>.

@lampajr lampajr force-pushed the label_values_java branch 4 times, most recently from e46a7e9 to d418e68 Compare September 16, 2024 13:24
@lampajr
Copy link
Member Author

lampajr commented Sep 17, 2024

With the latest 2 commits I did:

  • Moved the labelValues logic into a separated service - I think this is a great readability and maintainability improvement
  • Fixed the filter and multiFilter when the provided filter is a JSON object (which showed a great improvement wrt the current master branch when using a filter - will share some results later)
  • Applied the same improvement on RunService.labelValues call

There are still some TODOs:

  • Fix the logic when the provided filter is a JSON path - @johnaohara @willr3 do we want to keep supporting this use case? If not I won't spend time on this - the only solution I see is to decompose the jsonpath and extracting the root.
  • Fix the ordering by jsonpath - this is not working anymore as we have not the aggregated json in the query, @johnaohara @willr3 is this something we want to keep supporting? with this new implementation I don't even think this is still possible as this kind of ordering can be done in Java code once we aggregated JSON node - therefore we cannot use the psql jsonpath 🫤 As agreed this won't be supported any longer, just start/stop ordering other than the default by runId (or datasetId for labelValues for specific run)

@lampajr lampajr force-pushed the label_values_java branch 4 times, most recently from 446a978 to 3b83904 Compare September 19, 2024 16:37
@lampajr lampajr marked this pull request as ready for review September 19, 2024 16:58
@lampajr
Copy link
Member Author

lampajr commented Sep 19, 2024

@willr3 @johnaohara I am not fully satisified about the dirty trick I had to do in order to decompose the jsonpath for the jsonpath filter but it looks working.

I did some other performance comparions between current master branch and this PR, I attached the main important ones here 👉 https://issues.redhat.com/browse/MPL-455.

I am still a bit surprised about the x10 improvement in the case of filter obj (I'd like to investigate a bit more to ensure the response is actually the same - it was in my local env).

Anyway, I think this PR is ready for review and it comes with good improvements IMO but happy to get any feedback!

[edit] I kept different commits to highlight the different changes, I will squash them before merging if no other changes are required

Copy link
Member

@johnaohara johnaohara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johnaohara
Copy link
Member

@willr3 do you want to review this PR, looks good to me, but would be good to get a 2nd opinion as it is a big change to labelvalues

@willr3
Copy link
Collaborator

willr3 commented Oct 1, 2024

@willr3 do you want to review this PR, looks good to me, but would be good to get a 2nd opinion as it is a big change to labelvalues

taking a look

Copy link
Collaborator

@willr3 willr3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only part we need to clarify at the moment is if this change still works with jsonpaths that start with a full depth search (e.g. $..name). If it does not then we need to prevent those from being used and return an appropriate error

@lampajr

This comment was marked as resolved.

@lampajr
Copy link
Member Author

lampajr commented Oct 2, 2024

I rebased the PR and now some tests are failing, checking..

Fixed now!

@lampajr lampajr requested a review from willr3 October 2, 2024 11:23
Copy link
Collaborator

@willr3 willr3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@johnaohara johnaohara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went to merge this PR, but realised that there is an implementation detail leaking from the API classes. I think we should fix that before merging

I was double checking that no API changes were required so that it could be back-ported

@johnaohara
Copy link
Member

@lampajr thanks for sorting the api issue, please squash commits and I will merge

@lampajr
Copy link
Member Author

lampajr commented Oct 6, 2024

@lampajr thanks for sorting the api issue, please squash commits and I will merge

Here we go, commits squashed @johnaohara 🚀

@johnaohara johnaohara merged commit 27295df into Hyperfoil:master Oct 6, 2024
3 checks passed
@lampajr lampajr deleted the label_values_java branch October 6, 2024 08:56
@lampajr lampajr linked an issue Oct 16, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

return an error indication for invalid /labelValues filter parameters
3 participants